OpenAPI oneOf support #647

CharlesDuboisSAP · 2024-11-21T14:48:02Z

Context

oneOf / anyOf / allOf with multiple possible options should form an interface or a class that is able to parse those multiple options
openAPI templates were updated to 7.8.0

Feature scope:

oneOf support

Definition of Done

Functionality scope stated & covered
Tests cover the scope above
~~Error handling created / updated & covered by the tests above~~
Documentation updated Added OpenAPI generator options generateApis and generateModels
Release notes updated

...openapi-api-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/sample/model/Order.java

...anyof-oneof/output/com/sap/cloud/sdk/services/anyofoneof/model/RootObjectQuestionsInner.java

...pi-generator/src/test/resources/DataModelGeneratorIntegrationTest/oneof/input/sodastore.yaml

…able

newtork · 2024-11-26T09:06:47Z

...ugin/src/main/java/com/sap/cloud/sdk/datamodel/openapi/generator/DataModelGeneratorMojo.java

+    /**
+     * Generate model classes. Default is true.
+     */
+    @Parameter( property = "openapi.generate.generateModels", defaultValue = "true" )
+    private boolean generateModels;
+
+    /**
+     * Generate API classes (client classes). Default is true.
+     */
+    @Parameter( property = "openapi.generate.generateApis", defaultValue = "true" )
+    private boolean generateApis;
+


(Question)

Why not have this as separate PR? It's unrelated to oneof / anyof.

to save time

...ator/src/test/resources/DataModelGeneratorIntegrationTest/generate-apis/input/sodastore.yaml

newtork · 2024-11-26T09:15:46Z

...st/java/com/sap/cloud/sdk/datamodel/openapi/generator/DataModelGeneratorIntegrationTest.java

+            true,
+            6,
+            Map.of("useOneOfInterfaces", "true")),
+        GENERATE_APIS(


(Comment)

Not really sure, whether it's good to mix testing scopes of generate-api and anyof/allof/oneof.
But I'm not objecting at this point. You're just reusing the same spec file.

newtork · 2024-11-26T09:19:30Z

release_notes.md


 - Add support for `TypeDefinition` entries in OData V4 EDMX files.
+- OpenAPI generator improvements:
+  - Add `oneOf`, `anyOf` and `allOf` support for OpenAPI generation.


(Major)

I'll check the PR in AI SDK, but still it would be good to have a proper unit test in this repo, checking whether serialization and deserialization works at runtime. We are not even testing whether compilation works, right?

Otherwise, looks good!

Checking for compilation in the sample

Also this is a hidden feature in the additional parameters. It should receive the same tests as your pojoBuilder

Checking for compilation in the sample

Please link the sample that is being compiled.

this is a hidden feature in the additional parameters

I think this is incorrect, "enableOneOfAnyOfGeneration" is a well documented, publicly suggested and already used feature. It's not considered hidden compared to the pojoBuilder.

If we claim new and better support for oneof, anyof and allof - then we should have them tested for deserialization/serialization. If I see correctly the openapi-api-sample module only tests for allof currently.

I didn't change enableOneOfAnyOfGeneration but I added useOneOfInterfaces

You can see that the existing Test/input-spec-with-anyof-oneof/RootObjectQuestionsInner.java didn't change

It's somewhat odd to add useOneOfInterfaces in addition to enableOneOfAnyOfGeneration. It somewhat hides this new functionality from users. How much of a breaking change would it be to always enable the interfaces when enableOneOfAnyOfGeneration is defined?

Also agreeing with @newtork: We need tests in the sample code to ensure compilation actually works. Integration testing this isn't sufficient.

I added a deserialization test in the sample app

MatKuhr

some comments, most importantly though I think we need a compilation and a runtime check, i.e. sample code and test assertions on (de-) serialization

...or/src/test/resources/DataModelGeneratorIntegrationTest/generate-apis/output/test/OneOf.java

...ator/src/test/resources/DataModelGeneratorIntegrationTest/generate-apis/input/sodastore.yaml

...odelGeneratorIntegrationTest/generate-apis/output/test/OneOfWithDiscriminatorAndMapping.java

...anyof-oneof/output/com/sap/cloud/sdk/services/anyofoneof/model/RootObjectQuestionsInner.java

MatKuhr · 2024-11-26T12:45:56Z

...pi-generator/src/test/resources/DataModelGeneratorIntegrationTest/oneof/input/sodastore.yaml

@@ -0,0 +1,63 @@
+openapi: 3.0.0
+info:


why do we have this spec file twice now? Also, why not re-use the existing DataModelGeneratorIntegrationTest/input-spec-with-anyof-oneof/input/AggregatorNestedSchemaChild.json ?

I know we already have a rather bloated test setup here, with much duplication, but adding more to it will cause PR diffs to get larger and larger..

newtork · 2024-11-27T12:31:58Z

datamodel/openapi/openapi-api-sample/pom.xml

 				</executions>
 				<configuration>
-					<inputSpec>${project.basedir}/src/main/resources/sodastore.json</inputSpec>
+					<inputSpec>${project.basedir}/src/main/resources/sodastore.yaml</inputSpec>


(Question)

Why did you change from json to yaml?
Only for flavor, or does anyof/oneof not work for json?

Flavor, the file is shorter

...e/src/test/java/com/sap/cloud/sdk/datamodel/openapi/sample/api/OneOfDeserializationTest.java

Co-authored-by: Alexander Dümont <[email protected]>

Added tests

MatKuhr

Otherwise lgtm

MatKuhr · 2024-11-27T15:09:11Z

.../java/com/sap/cloud/sdk/datamodel/openapi/sample/model/OneOfWithDiscriminatorAndMapping.java

+/**
+ * OneOfWithDiscriminatorAndMapping
+ */
+


Ideally, this shouldn't be there, since it leads to "@type" fields during serialisation. In the AI SDK this is manually suppressed via mixins. It would be better to not generate this or mark it as disabled. Regardless of what we do, probably a serialisation test would be good

OpenAPI oneOf support

578b214

CharlesDuboisSAP self-assigned this Nov 21, 2024

CharlesDuboisSAP added please review Request to review a pull request do not merge Pull request must not be merged labels Nov 21, 2024

newtork reviewed Nov 25, 2024

View reviewed changes

...openapi-api-sample/src/main/java/com/sap/cloud/sdk/datamodel/openapi/sample/model/Order.java Outdated Show resolved Hide resolved

newtork reviewed Nov 25, 2024

View reviewed changes

...anyof-oneof/output/com/sap/cloud/sdk/services/anyofoneof/model/RootObjectQuestionsInner.java Outdated Show resolved Hide resolved

newtork reviewed Nov 25, 2024

View reviewed changes

...pi-generator/src/test/resources/DataModelGeneratorIntegrationTest/oneof/input/sodastore.yaml Outdated Show resolved Hide resolved

CharlesDuboisSAP added 9 commits November 25, 2024 09:54

Added USE_ONE_OF_INTERFACES true as default, revert non required=null…

6173eaa

…able

Added more cases in oneOf spec

1bfe9bc

Fix test

4d01a85

Remove warning

bd14470

Revert checkstyle changes

fedc13f

Removed JsonTypeName

2f1d938

Added generateApis option

f933754

Added javadoc

fe47d04

Added release notes

3d9c948

CharlesDuboisSAP enabled auto-merge (squash) November 26, 2024 08:16

CharlesDuboisSAP requested a review from newtork November 26, 2024 08:25

newtork reviewed Nov 26, 2024

View reviewed changes

...ator/src/test/resources/DataModelGeneratorIntegrationTest/generate-apis/input/sodastore.yaml Outdated Show resolved Hide resolved

newtork reviewed Nov 26, 2024

View reviewed changes

Fixed tests

834375f

MatKuhr previously requested changes Nov 26, 2024

View reviewed changes

CharlesDuboisSAP added 5 commits November 26, 2024 14:07

Removed useOneOfInterfaces from release notes

45398d1

Updated integration test

64173e3

Fixed spec

51e083e

Fixed spec

0a7ec0e

Added sample deserialization test

18336c8

newtork reviewed Nov 27, 2024

View reviewed changes

...e/src/test/java/com/sap/cloud/sdk/datamodel/openapi/sample/api/OneOfDeserializationTest.java Show resolved Hide resolved

newtork reviewed Nov 27, 2024

View reviewed changes

...e/src/test/java/com/sap/cloud/sdk/datamodel/openapi/sample/api/OneOfDeserializationTest.java Show resolved Hide resolved

Apply suggestions from code review

676031b

Co-authored-by: Alexander Dümont <[email protected]>

newtork previously approved these changes Nov 27, 2024

View reviewed changes

CharlesDuboisSAP added please merge Request to merge a pull request and removed do not merge Pull request must not be merged labels Nov 27, 2024

CharlesDuboisSAP added 2 commits November 27, 2024 13:48

Merge branch 'main' into oneOf

3dcd5a9

format

ec30ac4

CharlesDuboisSAP dismissed newtork’s stale review via ec30ac4 November 27, 2024 12:49

CharlesDuboisSAP requested a review from newtork November 27, 2024 13:24

MatKuhr approved these changes Nov 27, 2024

View reviewed changes

CharlesDuboisSAP merged commit 7e010e4 into main Nov 27, 2024
13 checks passed

CharlesDuboisSAP deleted the oneOf branch November 27, 2024 15:13

MatKuhr mentioned this pull request Dec 2, 2024

feat: [OpenAPI] Improved OneOf Support #657

Merged

10 tasks

OpenAPI oneOf support #647

OpenAPI oneOf support #647

Uh oh!

Conversation

CharlesDuboisSAP commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Feature scope:

Definition of Done

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

newtork Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newtork Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newtork Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharlesDuboisSAP Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatKuhr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MatKuhr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CharlesDuboisSAP commented Nov 21, 2024 •

edited

Loading

newtork Nov 26, 2024 •

edited

Loading

newtork Nov 26, 2024 •

edited

Loading

newtork Nov 26, 2024 •

edited

Loading

CharlesDuboisSAP Nov 26, 2024 •

edited

Loading